Comm Component for Simplex #3998
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the communication component for Simplex by adding new fields to the Config struct and implementing a Comm type for sending and broadcasting messages among validators. Key changes include:
- Adding new Config fields (Sender and OutboundMsgBuilder) to support message construction and delivery.
- Implementing Comm with methods for SendMessage and Broadcast.
- Updating tests and mocks to validate the communication flow and BLS signing functionality.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| simplex/test_utils.go | Added test utilities for validator info and engine config |
| simplex/config.go | Extended Config struct to include Sender and MsgBuilder |
| simplex/comm_test.go | Added tests for Comm functionality and error conditions |
| simplex/comm.go | Implemented Comm with SendMessage and Broadcast methods |
| simplex/bls_test.go, simplex/bls.go | Added BLS authentication with signing and verification |
| message/simplex_outbound_msg_builder.go | Added Simplex-specific outbound message builder functions |
| message/outbound_msg_builder.go | Integrated Simplex builder into general outbound messaging |
| message/messagemock/outbound_message_builder.go | Updated mocks with Simplex message methods |
| go.mod | Updated dependency for simplex package |
Signed-off-by: Sam Liokumovich <65994425+samliok@users.noreply.github.com>
Signed-off-by: Sam Liokumovich <65994425+samliok@users.noreply.github.com>
| @@ -181,6 +181,8 @@ type OutboundMsgBuilder interface { | |||
| chainID ids.ID, | |||
| msg []byte, | |||
| ) (OutboundMessage, error) | |||
|
|
|||
| SimplexOutboundMessageBuilder | |||
| } | |||
|
|
|||
| type outMsgBuilder struct { | |||
There was a problem hiding this comment.
We can utilize this in tests by using message.NewCreator.
simplex/comm.go
Outdated
| // nodes are the IDs of all the nodes in the subnet | ||
| nodes []simplex.NodeID |
There was a problem hiding this comment.
If we stored a copy of these as a set.Set[ids.ID] (without our own nodeID) then we wouldn't need to construct that set on every call to Broadcast.
There was a problem hiding this comment.
i think this would make things awkward with comm.Nodes() since it expects []simplex.NodeID and also expects the current nodeID to be included.
There was a problem hiding this comment.
I was suggesting to store both on the struct
Signed-off-by: Sam Liokumovich <65994425+samliok@users.noreply.github.com>
Why this should be merged
The
Comminterface is used by simplex to send and broadcast messages. This PR builds off pr #3993 and should be merged afterHow this works
This PR adds two new fields to the Config struct—
SenderandOutboundMsgBuilder—which enable the Simplex engine to construct and send outbound messages to other nodes.How this was tested
Using generated mocks for
ExternalSenderandOutboundMessageBuilder, I added tests in comm_test to verify that the correct messages are sent the expected number of times.Need to be documented in RELEASES.md?
No